[ASLayout] Revisit the flattening algorithm #395
Conversation
- Remove flattened flag - No more self check - Stop traversing a layout tree branch when hits a displaynode node. - Reuse as many existing ASLayout objects as possible
Source/Layout/ASLayout.mm
Outdated
| @@ -207,7 +215,10 @@ - (void)setRetainSublayoutLayoutElements:(BOOL)retainSublayoutLayoutElements | |||
|
|
|||
| - (ASLayout *)filteredNodeLayoutTree | |||
| { | |||
| NSMutableArray *flattenedSublayouts = [NSMutableArray array]; | |||
| if (ASLayoutIsFlattened(self)) { | |||
| self.retainSublayoutLayoutElements = YES; | |||
There was a problem hiding this comment.
This seems like a strange side effect. Should we document why this needs to be the case?
Source/Layout/ASLayout.mm
Outdated
| if (ASLayoutIsDisplayNodeType(layout)) { | ||
| if (sublayoutsCount > 0 || CGPointEqualToPoint(absolutePosition, layout.position) == NO) { | ||
| // Only create a new layout if the existing one can't be reused, which means it has either some sublayouts or an invalid absolute position. | ||
| layout = [ASLayout layoutWithLayoutElement:layout.layoutElement |
There was a problem hiding this comment.
Is this the "aggressive reuse"? Is this case skipped often enough in practice to make a difference? Not sure of the performance impact of removing the flag but do we have a good guess as to the consequences of landing this PR?
|
🚫 CI failed with log |
nguyenhuy
force-pushed
the
HNRemoveFlattenedFlag
branch
from
3dc7a04
to
75bbb17
Compare
|
@garrettmoon Good points! I run this diff against Pinterest master and got some numbers. It was able to reuse more than 140 Regarding performance impact of this diff, turns out it is 20-25% faster than the current implementation. Since each run is already very fast (less than 0.1ms) and can occur on background, the performance impact on each run is not noticeable, but can reach 100ms after ~7000 runs. That being said, I'm happy that this diff is not slower and any performance gain is just nice-to-have. Before After Here is the code I used to measure: nguyenhuy@b6ff4ed. Results got from an iPhone 6S, iOS 10.3.2. |
|
That's great! Thanks for doing the perf checks! |
|
Landing this, thanks @garrettmoon and @maicki! @Adlai-Holler Feel free to do a post-merge review here. |
* Implement tests for the layout flattening process * Refactor the flattening algorithm - Remove flattened flag - No more self check - Stop traversing a layout tree branch when hits a displaynode node. - Reuse as many existing ASLayout objects as possible * Update changelog * Ceil position values before comparing * Explain why sublayout elements must be retained
* Implement tests for the layout flattening process * Refactor the flattening algorithm - Remove flattened flag - No more self check - Stop traversing a layout tree branch when hits a displaynode node. - Reuse as many existing ASLayout objects as possible * Update changelog * Ceil position values before comparing * Explain why sublayout elements must be retained
I'm facing an issue that blocks the first release of Weaver. In order to inspect the whole layout element tree that contains both display nodes and layout specs, I have to tell all display nodes to skip flattening their calculated layout immediately and only do so when the layout is applied (#337). When it's time to flatten a layout tree, the tree contains unflattened calculated layouts of all nodes in the corresponding node tree. The flattening algorithm doesn't handle this use case well because it relies on
isFlattenedflag of eachASLayoutobject which is never turned on.The fact that the algorithm relies on that flag also caused a problem with Yoga integration because a Yoga tree is always flattened, but we may forget to set the flag. If we take a step back, this flag is actually a "calculated" flag which means it is determined by other internal states and shouldn't be a stored property.
This PR aims to eliminate the flag completely. There was a performance concern last time @maicki tried to remove it (facebookarchive/AsyncDisplayKit#3039). I took extra steps to ensure that we're now reusing
ASLayoutobjects aggressively.Besides the flag elimination, there are some other changes:
context.layoutagainstself.if-elseclause here). This is to make sure that, even if a sublayout is not flattened, no subnode of that sublayout is ever considered a direct subnode of the root node.rootLayout.sublayouts[i].sublayouts.countis always 0. (Change here)